fix(engine): preserve source frame identity above 99,999 - #3503

R1 adversarial review — the in-scope fix is solid; flagging one sibling-path scope gap.

Fix — correctness

The parse-by-ordinal helper (extractedFrameIndex.ts) is the right shape. Points I verified adversarially:

• Boundary math — frame_99999.jpg → index 99_998 and frame_100000.jpg → index 99_999 tests pin down the exact off-by-one that would otherwise re-introduce the bug on refactor. Number.isSafeInteger guards ludicrously wide digit runs; < 1 rejects zero-based writers.
• Backward compat — the writer is unchanged (frame_%05d.${format} — ffmpeg's %05d grows past 5 digits, doesn't wrap). New reader accepts 5-digit and 6-digit filenames since it parses the digit run rather than the padded width. Existing caches on disk keep working.
• Missing-frame detection (framePathsFromDirectory lines 44–52) iterates 0..indexed.size-1 and requires every slot filled — a hole at index N with a later frame present raises "Missing extracted frame index N" rather than silently truncating. Covered by test.
• Duplicate detection — frame_1.jpg + frame_00001.jpg both parse to index 0 and throw. Extractor's own writer always pads to 5 so this is defensive, but the assertion is right.
• Sentinel/other files — the startsWith(FRAME_FILENAME_PREFIX) && endsWith(suffix) filter leaves .hf-complete and stray files alone.

Tests hit semantics (specific index values, specific error strings), not just presence — mutation-resistant.

Scope gap — worth a follow-up

packages/producer/src/services/distributed/renderChunk.ts:361-378 (rebuildExtractedFramesFromPlanDir) has the identical lex-sort assumption on the same source-video frames for the dense-v1 index mode (the parameter default, and the branch taken whenever v2Manifest === null at renderChunk.ts:665). The comment above the block still asserts:

Sorted-by-name matches sorted-by-frame-index because the extractor writes zero-padded monotonic indices.

That claim is exactly what this PR contradicts for ≥100_000 frames. Distributed renders without a v2 manifest that consume source video ≥100_000 extracted frames will silently produce the same misaligned identity bug you're fixing here.

Recommendation: either (a) route dense-v1 through framePathsFromDirectory too, or (b) if dense-v1 is being retired, document that plan and add a runtime guard rejecting frames.length >= 100_000 under dense-v1.

Not blocking this PR — the local + cache-rehydrate paths are the ones the ticket targets, and the fix is clean. But worth pulling into an immediate follow-up so the invariant "source-frame identity is derived from ffmpeg ordinal, never from lex position" holds monorepo-wide.

— Via

🟢 Real fix, targeted diff, good defensive test set. No blockers. One meaningful adjacent-scope concern to surface: the same lexical-sort bug lives in the distributed producer's dense-v1 rehydrate path — outside this PR's scope, worth a follow-up ticket. Details below.

What I verified (HEAD a290fdfd)

• Reader is ordinal-derived, not width-hardcoded. extractedFrameIndex at packages/engine/src/services/extractedFrameIndex.ts:15-25 parses frame_(\d+)\.<format>$ — any digit count parses cleanly, so frame_99999.jpg and frame_100000.jpg land at indexes 99998/99999 (verified by the test at extractedFrameIndex.test.ts:29-32).
• Two-pass contiguity check is correct. First pass collects into indexed and catches duplicates; second pass iterates 0..indexed.size-1 and catches gaps (extractedFrameIndex.ts:32-53). I walked several gap shapes (1,3,4,5 → gap-1; 2,3,4 → missing-0; 1..5 + 7 → gap-6) mentally — all fail as expected.
• Writer format unchanged. frame_%05d.<ext> still emitted at videoFrameExtractor.ts:700 (direct extract) and 1184 (superset), and frameFileName still uses padStart(5, "0") at videoFrameExtractor.ts:1199. %05d is printf-style minimum-width, so ordinals ≥ 100000 spill to 6 digits — which is exactly the bug this PR fixes. Cache dirs written by earlier engine versions remain readable (same emit pattern, new reader tolerates both widths).
• Round-trip is consistent. FFmpeg's -start_number defaults to 1 (no override anywhere on the extract path — chunkEncoder.ts:579 is the only -start_number site and is on the encode side, different dir). Reader subtracts 1 to produce 0-based indexes. Superset-slice writer at videoFrameExtractor.ts:1346 re-emits with frameFileName(i + 1, format), matching the 1-based on-disk convention.
• All three original callers of the sort-based reader are migrated. videoFrameExtractor.ts:806, videoFrameExtractor.ts:1185, and extractionCache.ts:512 all now call framePathsFromDirectory. The old extractedFrameFileNames helper is deleted. Remaining readdirSync sites in extractionCache.ts (lines 184/337/448) are unrelated (GC + "has-any-frame" probe — order-independent).
• No sibling consumers. Grepped hyperframes-internal, hyperframes-gemini-agent, pacific, heygen-cli, experiment-framework, genesis for FRAME_FILENAME_PREFIX / framePathsFromDirectory / frame_%05d — no hits. This is engine-local naming, no cross-repo contract to break.

Concerns (🟡)

• renderChunk.ts:343 has the same bug on dense-v1 — out of scope, follow-up ticket. rebuildExtractedFramesFromPlanDir in packages/producer/src/services/distributed/renderChunk.ts:343-394 is a parallel implementation used by the distributed producer's chunk workers when reading plan-dir frames. Default indexMode: "dense-v1" uses readdirSync().sort() and maps by sort-position (line 362-374). The comment at 358 even acknowledges the fragile invariant: "Sorted-by-name matches sorted-by-frame-index because the extractor writes zero-padded monotonic indices." That invariant breaks at 100k+ frames for the same reason as engine's cache path. sparse-v2 is fine — it parses ordinals via /(\d+)(?=\.[^.]+$)/ at line 372. dense-v1 is chosen when v2Manifest === null (renderChunk.ts:665), i.e. it's the fallback for plans without a v2 manifest — actively in use for legacy plans. Distributed renders long enough to cross 99,999 source frames (e.g. > ~55 min at 30 fps on dense-v1 plans) will exhibit the same interleave bug there. Recommend a follow-up PR that either migrates dense-v1 to the same ordinal-derived path or forces the boundary invariant. Explicitly NOT a blocker for this PR — engine-scoped fix is right — but should not be lost.
• planV2.ts:308 uses a sparse-tolerant reader, this one is strict-contiguous. listVideoFramePaths in packages/producer/src/services/distributed/planV2.ts:308-345 derives ordinals the same way but allows sparse indexes ("Preserve sparse indexes so a materialized chunk can carry only the frames it actually requests" — line 318-319). New framePathsFromDirectory requires 0..N-1 contiguous. Different use cases justify the divergence (chunk-worker reads sparse-materialized dirs; engine reads full-extraction dirs), but I'd want a code-adjacent comment or shared prefix parser saying "sparse OK for producer, strict for engine" so a future refactor doesn't accidentally unify them.
• Fresh-extraction path stays hard-fail even when partial frames were written. videoFrameExtractor.ts:806 calls framePathsFromDirectory immediately after a successful ffmpeg exit. If ffmpeg reports success but skipped an intermediate frame (rare — corrupted keyframe, filesystem hiccup, container quirk), the pre-existing zero_output guard at 807-814 only catches size==0. With the new reader, non-contiguous partial extraction now throws ExtractedFrameSequenceError — good (previously it silently returned wrong frames), but the caller's classifier (classifyVideoExtractionError, called from extractionError at 1691) doesn't recognize this error class and will surface it as unknown_error/ffmpeg_failed shape rather than something the retry loop understands. Worth a quick check that runVideoExtractionWithRetry doesn't loop forever on a genuinely unretriable "your ffmpeg dropped a frame" case, and that operators get a clean diagnostic string.
• Cache-hit hard-fail can crash a render on legacy corruption. rehydrateCacheEntry throws → rehydratePublishedCache returns the throw → lookupCacheFor doesn't catch it → extractAllVideoFrames bubbles it up. That's correct for surfacing bad data instead of silently rendering wrong frames, but on rollout, any pre-existing dirty cache entry (partial write from a crashed render, orphan file, etc.) that used to silently work-wrong will now hard-fail the next render that hits it. A graceful-degrade path — catch ExtractedFrameSequenceError from rehydrate*, evict the entry, log, fall through to a fresh extraction — would keep users unblocked while surfacing the anomaly in logs. Judgment call; fail-loud is defensible. Flag with the on-call so they know cache-related render failures may spike briefly post-deploy.

Nits

• new RegExp(...) allocated per file inside framePathsFromDirectory's loop (via extractedFrameIndex at line 16). For a 100k-frame dir that's 100k RegExp allocations. Hoist to a Map<ExtractedFrameFormat, RegExp> module-const. Cheap win, harmless.
• Test refuses malformed, zero, and wrong-format frame candidates seeds frame_00000.jpg as invalid. Correct per FFmpeg's 1-based default, but the assertion is testing something no real extractor writes. Fine defensively; a comment tying it to -start_number = 1 would help the next reader.
• Duplication with packages/producer/src/services/distributed/planV2.ts:308-345 is now more visible. Long term, exporting extractedFrameIndex from a shared location that both engine and producer import would consolidate — but the diff scope here is right to defer.

Tests — what's covered, what's not

Covered:

• Per-file ordinal derivation across the 5→6 digit boundary (frame_99999.jpg / frame_100000.jpg) at extractedFrameIndex.test.ts:29-32.
• Malformed, zero-ordinal, wrong-extension rejections (extractedFrameIndex.test.ts:34-42).
• Full-directory ordinal mapping with lexical scramble (10 files, reversed) at extractedFrameIndex.test.ts:46-58.
• Duplicate-ordinal rejection via mixed-width filenames (frame_1.jpg + frame_00001.jpg) at 61-66.
• Gap rejection (frame_00001.jpg + frame_00003.jpg) at 68-73.
• Frame-prefixed malformed rejection with unrelated files ignored at 75-80.
• Cache-rehydrate gap rejection at extractionCache.test.ts:79-115.

Not covered — worth adding at least one:

• Full-directory mapping across the actual 5→6 digit boundary. The whole point of the PR. Seed a dir with e.g. frame_99998.jpg, frame_99999.jpg, frame_100000.jpg, frame_100001.jpg, run framePathsFromDirectory, and assert basename(paths.get(99999)) is frame_100000.jpg (not frame_99999.jpg which is what the old lex-sort would have produced). Right now the boundary is tested per-file but not through the map builder — the highest-value regression test for this specific bug is missing.
• Cache-rehydrate happy-path across the boundary. Same idea, at the rehydrateCacheEntry level.
• Superset-slice happy-path across the boundary — a sliceSupersetMember call where offsetFrames + i crosses 99999. Long, but tractable with fake framePaths.

Adversarial ledger

• Mixed-width caches. ✅ Correctly handled. Reader is width-agnostic; writer's %05d produces 5+ digits monotonically. A cache written entirely at ≤99999 stays 5-digit; one that extended to ≥100000 has both widths and the ordinal-parser stitches them into the right order.
• Extremely long renders. JS Number.MAX_SAFE_INTEGER is 2^53-1. Number.isSafeInteger gate at line 21 covers overflow. A 24hr render at 30fps is 2.6M frames — well within safe-integer bounds. Not a concern.
• Ordinal overflow. Same story — safe-integer gate is present.
• FFmpeg version behavior differences. %05d in image2 muxer is documented printf-style minimum-width; I've not experimentally re-verified against the exact ffmpeg version this project pins, but the assumption is standard and consistent with pre-existing engine code. If the pinned ffmpeg ever changed to a fixed-width interpretation, extraction would fail loudly with the new reader (out-of-range ordinal on frame_00000.jpg if it wrapped, or a filename that regex-matches something bizarre) — better failure mode than the previous silent misorder. No PR change required.
• Cache dir with a stale .hf-complete sentinel and no frames. Reader returns an empty map; caller at videoFrameExtractor.ts:807 catches the zero-output case (though only for direct extraction, not rehydrate). Rehydrate with an empty complete dir would return totalFrames: 0 — same as before this PR, not a regression. Existing pre-PR sentinel-guard behavior at dae5b7b90 handles that.
• Producer dense-v1 chunk dir with 100k+ frames. ❌ Not fixed here (out of scope). See the top concern.
• Two independent extractions racing into the same dir. Not addressed by this PR (nor should be — cache directory locking is a separate concern). Duplicate-ordinal detection would surface it as a hard error rather than silent corruption, which is a strict improvement.

Stamp stance

🟢 LGTM from my side — leaving as a comment. The engine-scoped fix is right, the reader is sound, and the test coverage for the failure modes is thorough (with the one boundary-through-map-builder gap noted above). Miguel decides merge; the follow-up on renderChunk.ts dense-v1 should be a separate ticket, not a blocker here.

— Review by Rames D Jusso

Follow-up verified at HEAD c45c8a29fc428a629009d443f09f897b432d4d68 — closes Concern (1) from #3503 (review).

Delta from a290fdfd → c45c8a29 (2 files, +46/-6):

• packages/producer/src/services/distributed/renderChunk.ts — new helper frameNumberFromFileName(name): number | null (regex /(\d+)(?=\.[^.]+$)/, safe-int gated) at 327-332. rebuildExtractedFramesFromPlanDir now sorts the directory listing with numeric-compare via the helper, falling back to localeCompare on unparseable names or as tie-breaker; inline regex previously used only for sparse-v2 decrement is also refactored onto the helper.
• packages/producer/src/services/distributed/rebuildExtractedFrames.test.ts — new test orders mixed-width dense-v1 filenames by numeric ordinal: creates frame_1.jpg through frame_10.jpg, feeds .toReversed() to simulate the adversarial lex-sort input, asserts framePaths.get(8) == frame_9.jpg and framePaths.get(9) == frame_10.jpg. Uses the 1→2 digit boundary — same mechanism as 5→6, cheaper to construct.

What this closes

• dense-v1 distributed renders past the width boundary (frame > 99,999 with %05d writer) now stitch into a contiguous ordinal-ordered sequence rather than the interleaved lex-sort. Semantics for the well-formed monotonic-width case are byte-identical (numeric-sort is stable and matches lex-sort when widths align).
• sparse-v2 semantics preserved — still frameNumber - 1 decrement (1-based ffmpeg → 0-based index) via the same helper.

Non-blocker note — the localeCompare fallback when either name can't be parsed means a stray non-numeric file in the frames directory would push the whole sort back to lex among that batch. The .filter(endsWith(ext)) already narrows the set, so realistic exposure is bounded; noting for the follow-up round rather than this PR.

Stamp stance — 🟢 no further concerns from this side. Test claim (612/612 producer unit + 5/5 focused contract + lint/format/Fallow clean) accepted on trust — the diff itself is safe on inspection.

Merge is @miguel-heygen's call.

— Review by Rames D Jusso

Delta clean — R1 addressed.

Delta since prior R1 head (a290fdfd→c45c8a2): 2 files, +46/-6 by @miguel-heygen.

R1 finding verified fixed — packages/producer/src/services/distributed/renderChunk.ts:

• frameNumberFromFileName() helper extracts the last numeric run before .ext ((\d+)(?=\.[^.]+$)), with a Number.isSafeInteger guard.
• Sort now numeric with lex tiebreak: leftNumber - rightNumber || left.localeCompare(right); falls back to localeCompare when either operand has no trailing digits, so non-numeric names don't throw or crash the sort.
• Dense-v1: positional indexing (frameIndex = i) preserved — zero-based v1 (frame_00000.jpg → get(0)) still correct because position 0 in numerically-sorted list is the smallest number.
• Sparse-v2: frameIndex = frameNumber - 1 preserved (1-based ffmpeg → 0-based key), now sharing the same digit-extraction helper as sort.

Regression test verified (rebuildExtractedFrames.test.ts:181):

• Writes 10 mixed-width filenames (frame_1.jpg…frame_10.jpg) in reversed order → exercises the exact bug shape (variable digit-width in the same directory).
• Asserts specific paths: get(8) === frame_9.jpg, get(9) === frame_10.jpg. Under the pre-fix lex sort this would resolve to frame_8.jpg / frame_9.jpg — so the test FAILS pre-fix and PASSES post-fix. Semantics-not-presence, per prior-invariant discipline.
• Existing tests preserved: zero-based v1 (frame_00000 → get(0)) still asserted; sparse-v2 mapping (frame_00021 → get(20)) still asserted using the shared helper.

Adversarial pass:

• (a) Non-frame .jpg files: extension filter keeps them, frameNumberFromFileName returns null on names without trailing digits, sort falls back to localeCompare — no throw, no regression (pre-existing indexing behavior for weird names retained).
• (b) FFmpeg patterns: %d produces unsigned decimal; \d+ regex only matches unsigned integers; Number.isSafeInteger guards against pathological >2^53 filenames.
• (c) Dense-v1 count/duplicate gate untouched — framePaths.size still equals frames.length for the dense mode, so downstream coverage-check semantics unchanged.

CI on c45c8a2 still spinning at review time (push at 15:50:33Z, WIP green). Approving on fix-verification per Magi's local lane report (producer-unit 612/612, focused contract 5/5, typecheck/lint/format/Fallow clean); will follow up if any lane surfaces a real regression.

— Via